Skip to content

Conversation

@colesbury
Copy link
Contributor

@colesbury colesbury commented Jan 12, 2026

Also fix "Install dependencies" step so that we use the installed Clang. We can use clang-20 on both TSan and UBSan now.

- Also fix "Install dependencies" step so that we use the installed Clang.
  We can use clang-20 on both ASan and TSan now.
@colesbury colesbury added tests Tests in the Lib/test dir topic-SSL labels Jan 12, 2026
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(early GHA feedback)

Comment on lines +26 to +31
strategy:
fail-fast: false
matrix:
os: [ubuntu-24.04]
openssl_ver: [3.5.4]
runs-on: ${{ matrix.os }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz move these to inputs: and host the matrix on the calling side (in build.yml). This is the separation I was going for when I first introduced the reusable-*.yml module pattern — no matrices in modules.

Comment on lines 23 to 25
&& ' (free-threading)'
|| ''
}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need to interpolate new matrix factors since the job names will be confusing if there's ever more than one job.

Comment on lines +58 to 59
run: |
if [ "${SANITIZER}" = "TSan" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move this conditional to the GHA step so that skipping is more prominent.

Suggested change
run: |
if [ "${SANITIZER}" = "TSan" ]; then
if: inputs.sanitizer == 'TSan'
run: |

Also drop fi and the second conditional block below too.

|| '--with-undefined-behavior-sanitizer'
}}
--with-pydebug
${{ inputs.sanitizer == 'TSan' && ' --with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit) no need for the whitespace since >- in YAML already adds whitespaces when it joins the lines on parsing.

Suggested change
${{ inputs.sanitizer == 'TSan' && ' --with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }}
${{ inputs.sanitizer == 'TSan' && '--with-openssl="$OPENSSL_DIR" --with-openssl-rpath=auto' || '' }}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir topic-SSL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants